GH-31318: [Python] Add fixed-offset timezones to Hypothesis test strategy - #49844
Conversation
|
|
|
|
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
|
Revision: 6c9a20e Submitted crossbow builds: ursacomputing/crossbow @ actions-16091e66e1
|
|
The extended hypothesis failure is currently unrelated, opened an issue for it here: #49846 |
AlenkaF
left a comment
AlenkaF
left a comment
There was a problem hiding this comment.
Thanks for the PR!
The only comment I have for now is that the current strategy can produce UTC out of the valid UTC range which is UTC-12 and UTC+14, for example UTC-12:45. Datetime will accept it but as this is invalid IANA timezone it should be improved, it also might produce errors in other parts.
|
|
2 similar comments
|
|
|
|
|
@AlenkaF thanks for the review! edit: on hindsight, a lower bound filter is unnecessary since the lower bound can never be violated -- harmless, clear and defensive, but dead code. |
|
Thanks for the update! There is a PR up to fix the hypothesis build: #49847. After that is merged you can rebase and we can run the build here, to double check the logic. |
|
@alex-anast the PR has been merged on main, if you could rebase I'll re-trigger the hypothesis tests on CI |
564317e to
820665d
Compare
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
|
expected 😓 @raulcd can you please re-trigger the test when you get the chance? |
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
|
Revision: 820665d Submitted crossbow builds: ursacomputing/crossbow @ actions-096afcf5c0
|
|
I think the failure is connected to the changes in this PR: TypeError: tzinfo argument must be None or of a tzinfo subclass, not type 'datetime.timedelta'see: https://github.com/ursacomputing/crossbow/actions/runs/24890383634/job/72880860677#step:6:3978 |
|
@raulcd sorry for the spam -- can you retrigger the workflow once more please? |
3227762 to
f0e060e
Compare
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
Don't worry at all, sorry I missed your message yesterday. |
|
Revision: f0e060e Submitted crossbow builds: ursacomputing/crossbow @ actions-4a0924ba7a
|
…pothesis health checks
|
Alright, I managed to reproduce the issue with: PYARROW_TEST_HYPOTHESIS=ON \
python -m pytest .venv/lib/python3.12/site-packages/pyarrow/tests/test_types.py::test_hashing \
-W error -x -v \
--hypothesis-seed=309228849660331397780595412479768595882 With the latest commit, the workflow passes locally. @raulcd if you could launch one more (hopefully final) time, please 🙏 The problems were that:
So now I'm using a precomputed list (no runtime construction and filtering) and the |
|
@github-actions crossbow submit test-conda-python-3.11-hypothesis |
|
Revision: a5fdd42 Submitted crossbow builds: ursacomputing/crossbow @ actions-5df144ad3d
|
github-actions
Bot
added
awaiting committer review
|
|
|
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 4373fdf. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 4373fdf. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
…t strategy (apache#49844) ### Rationale for this change The Hypothesis test strategy for timezones (`pyarrow.tests.strategies.timezones`) only generates named IANA timezones (via pytz and zoneinfo). It does not generate fixed-offset timezones like `+05:30` or `-03:00`. As noted in apache#31318, this is not supported out of the box by Hypothesis, so a custom strategy is needed. Fixed-offset timezones are already used in manual tests and handled correctly by Arrow, but are never exercised via property-based testing. ### What changes are included in this PR? Adds a `fixed_offset_timezones` strategy to `python/pyarrow/tests/strategies.py` that generates `datetime.timezone` objects with offsets ranging from UTC-12:00 to UTC+14:00, with minute components of 0, 30, or 45 (covering all real-world UTC offsets). This strategy is included in the existing `timezones` strategy across all dependency configurations. ### Are these changes tested? This change improves test infrastructure itself -- it extends the `timezones` strategy used by existing Hypothesis-based tests. The generated fixed-offset timezones were verified to work correctly with `pa.timestamp()`. The test command used was: ```sh export MAMBA_ROOT_PREFIX=$PWD/.micromamba && eval "$(/tmp/bin/micromamba shell hook -s bash)" && micromamba activate pyarrow-dev && export LD_LIBRARY_PATH=$CONDA_PREFIX/lib:$LD_LIBRARY_PATH && python -m pytest pyarrow/tests/test_scalars.py pyarrow/tests/test_array.py -v -k test_dunders ``` ### Are there any user-facing changes? No. This only affects the internal test suite. ### AI-generated code disclosure This PR was developed with assistance from an AI coding tool (Claude, Anthropic). All changes have been reviewed, understood, and verified. * GitHub Issue: apache#31318 Authored-by: Alexandros Anastasiou <anastasioyaa@gmail.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
3 participants
Rationale for this change
The Hypothesis test strategy for timezones (
pyarrow.tests.strategies.timezones) only generates named IANA timezones (via pytz and zoneinfo). It does not generate fixed-offset timezones like+05:30or-03:00. As noted in #31318, this is not supported out of the box by Hypothesis, so a custom strategy is needed. Fixed-offset timezones are already used in manual tests and handled correctly by Arrow, but are never exercised via property-based testing.What changes are included in this PR?
Adds a
fixed_offset_timezonesstrategy topython/pyarrow/tests/strategies.pythat generatesdatetime.timezoneobjects with offsets ranging from UTC-12:00 to UTC+14:00, with minute components of 0, 30, or 45 (covering all real-world UTC offsets). This strategy is included in the existingtimezonesstrategy across all dependency configurations.Are these changes tested?
This change improves test infrastructure itself -- it extends the
timezonesstrategy used by existing Hypothesis-based tests. The generated fixed-offset timezones were verified to work correctly withpa.timestamp().The test command used was:
Are there any user-facing changes?
No. This only affects the internal test suite.
AI-generated code disclosure
This PR was developed with assistance from an AI coding tool (Claude, Anthropic). All changes have been reviewed, understood, and verified.